Skip to content

Add cloud credential serialization package#18

Closed
devin-ai-integration[bot] wants to merge 2 commits intomainfrom
devin/1754700853-cloud-credential-serialization
Closed

Add cloud credential serialization package#18
devin-ai-integration[bot] wants to merge 2 commits intomainfrom
devin/1754700853-cloud-credential-serialization

Conversation

@devin-ai-integration
Copy link
Contributor

@devin-ai-integration devin-ai-integration bot commented Aug 9, 2025

Address PR feedback: refactor cloud credential serialization to internal/

Summary

This PR addresses the GitHub PR feedback on #18 by implementing three major refactoring changes to the cloud credential serialization package:

  1. Package relocation: Moved the serialization package from pkg/v1/ to internal/serialization/v1/ to make it internal-only
  2. Credential type reuse: Removed custom credential data structs and now directly use existing credential types from provider implementations (LambdaLabsCredential, FluidStackCredential, NebiusCredential)
  3. Provider ID constants: Replaced hardcoded provider ID strings with actual constants from provider packages (lambdalabsv1.CloudProviderID, fluidstackv1.CloudProviderID, "nebius")

The refactoring maintains all existing serialization functionality while improving code reuse and consistency. JSON tags were added to the existing credential structs to enable serialization without creating duplicate data structures.

Review & Testing Checklist for Human

  • Test end-to-end serialization/deserialization - Verify that real credential objects can be serialized to JSON/bytes/string and deserialized back correctly for all three providers
  • Verify JSON tags don't break existing code - Check that adding JSON tags to LambdaLabsCredential, FluidStackCredential, and NebiusCredential doesn't affect their usage elsewhere in the codebase
  • Confirm provider ID constants are correct - Verify that lambdalabsv1.CloudProviderID ("lambda-labs"), fluidstackv1.CloudProviderID ("fluidstack"), and "nebius" match the previously hardcoded strings
  • Check for circular import issues - Ensure the new cross-internal package imports don't create dependency cycles
  • Validate API surface change - Confirm that moving the package to internal/ aligns with the intended API design (package no longer available to external consumers)

Recommended test plan: Create credential objects for each provider, serialize them using the new package, deserialize them back, and verify the round-trip preserves all data correctly.


Diagram

%%{ init : { "theme" : "default" }}%%
graph TD
    subgraph "Old Structure (Deleted)"
        OldSer["pkg/v1/serialization.go"]:::major-edit
        OldTest["pkg/v1/serialization_test.go"]:::major-edit
    end
    
    subgraph "New Structure (Created)"
        NewSer["internal/serialization/v1/serialization.go"]:::major-edit
        NewTest["internal/serialization/v1/serialization_test.go"]:::major-edit
    end
    
    subgraph "Provider Packages (Modified)"
        Lambda["internal/lambdalabs/v1/client.go<br/>LambdaLabsCredential"]:::minor-edit
        Fluid["internal/fluidstack/v1/client.go<br/>FluidStackCredential"]:::minor-edit
        Nebius["internal/nebius/v1/client.go<br/>NebiusCredential"]:::minor-edit
    end
    
    subgraph "Core Interface (Context)"
        Core["pkg/v1/client.go<br/>CloudCredential interface"]:::context
    end
    
    NewSer -->|"imports & uses"| Lambda
    NewSer -->|"imports & uses"| Fluid
    NewSer -->|"imports & uses"| Nebius
    Lambda -->|"implements"| Core
    Fluid -->|"implements"| Core
    Nebius -->|"implements"| Core
    
    subgraph Legend
        L1[Major Edit]:::major-edit
        L2[Minor Edit]:::minor-edit
        L3[Context/No Edit]:::context
    end
    
    classDef major-edit fill:#90EE90
    classDef minor-edit fill:#87CEEB
    classDef context fill:#FFFFFF
Loading

Notes

  • All tests pass and lint checks are clean
  • The serialization package now has cross-internal dependencies on provider packages, following the pattern established by the validation package
  • JSON tags use snake_case naming convention consistent with the rest of the codebase
  • The SerializableCredential interface is preserved for backward compatibility but now works with actual credential structs

Link to Devin run: https://app.devin.ai/sessions/fee5d20f847747759938c50465de86cb
Requested by: Alec Fong (@theFong)

- Support serialization to/from bytes, string, and JSON formats
- Provider registry system for automatic credential reconstruction
- Comprehensive test coverage for all provider types
- Round-trip serialization/deserialization support
- SerializableCredential interface for extensible serialization
- Validation and error handling for all credential types

Co-Authored-By: Alec Fong <alecsanf@usc.edu>
@devin-ai-integration
Copy link
Contributor Author

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment and CI monitoring

Copy link
Member

@theFong theFong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this seems directionally right, however lets reuse the Credential data type in each of the internal/{provider} implementation. We can move this package into internal/


func DeserializeCredentialByProvider(providerID string, data json.RawMessage) (CloudCredential, error) {
switch providerID {
case "lambda-labs":
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lets use the actual provider ids found in the provider impl

- Move serialization package from pkg/v1/ to internal/serialization/v1/
- Add JSON tags to existing credential structs in provider packages
- Use actual provider ID constants instead of hardcoded strings
- Update deserialization to construct actual credential objects
- Remove old pkg/v1/serialization files
- Maintain all existing functionality and test coverage

Co-Authored-By: Alec Fong <alecsanf@usc.edu>
@theFong theFong closed this Aug 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant